Navigation: Handle block menu items #24846
Conversation
noisysocks
added
[Type] Bug
|
Size Change: +148 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
|
Looks like there's some legitimate E2E test failures. I'll look into them tomorrow. |
| if ( menuItem.classes?.length && some( menuItem.classes ) ) { | ||
| attributes.className = menuItem.classes.join( ' ' ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ideally we'd also set id and type here so that the URL can be dynamic. Need to think about these attributes some more, though. I've added a note to come back to this in #18345.
There was a problem hiding this comment.
I'm unclear on the reasoning behind dropping these attributes, what's the background?
There was a problem hiding this comment.
So the existing code in master has a few problems:
-
We're setting the
typeattribute to bemenuItem.type, but this is incorrect because the attribute should be one of'page','post','category','tag', orundefinedandmenuItem.typereturns'post_object'or'taxonomy'IIRC. -
We're setting
idtomenuItem.id, but this is incorrect because for a link to dynamically render its URL it will need the ID of the post, not the ID of the menu item. It should probably be set tomenuItem.object_id.
Instead of fixing these issues, though, I added a note to address them when we address #18345 as there's some thinking to be done around what attributes are needed for dynamically rendering a link. IMO we should be storing three things with the block: the menu item type, the object type, the object ID. See #18345 (comment).
There was a problem hiding this comment.
Thanks for the explanation, sounds like it's not something to tackle here.
It'd be good to rewrite the description on the issue with these details. 👍
|
I've updated the E2E test snapshots. This is ready for review. |
Handle menu items with type = 'block' when creating a Navigation block from an existing menu.
noisysocks
force-pushed
the
fix/handle-block-menu-items-when-creating-navigation-block
branch
from
da4bb5f to
5c64c4b
Compare
| innerBlocks | ||
| ); | ||
| if ( ! block ) { | ||
| return createBlock( 'core/freeform', { |
There was a problem hiding this comment.
While this seems to work in testing, I think it'd be safest to also add core/freeform to the allowedBlocks of the nav block innerBlocks component.
In testing you can actually hack any block into the inner blocks of another block using the code editor (is this a bug?), but probably best not to rely on this behavior.
There was a problem hiding this comment.
I do wonder if, instead, we should simply fail (loudly?) in this case. It's undefined behaviour for the menu item to have 0 or > 1 blocks in it and probably indicates a bug elsewhere. Thoughts?
There was a problem hiding this comment.
Oh right, I think I've misunderstood what it's for. It actually looks like parse gives you a freeform block anyway if it's valid HTML that doesn't contain a block.
I think you're right, it should probably fail, but in a way that still makes the navigation recoverable. Not sure what the best option would be.
| if ( menuItem.classes?.length && some( menuItem.classes ) ) { | ||
| attributes.className = menuItem.classes.join( ' ' ); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm unclear on the reasoning behind dropping these attributes, what's the background?
talldan
deleted the
fix/handle-block-menu-items-when-creating-navigation-block
branch
Fixes #24675. Fixes #25087.
Updates the Navigation block to properly convert menu items with
type = 'block'when creating a Navigation block from an existing menu.I also made it pull the
description,relandclassNameover from the menu item.How to test
Add a Search block to a menu using the Navigation screen. See Navigation screen: Add opt-in Navigation block rendering #24503 for instructions.
Save the menu.
Go to a post and insert a Navigation block.
Select the menu and click Create.
Note that the Search block is properly added.